Use squeeze instead of gsub in SquishNormalizer #53
Conversation
| @@ -2,7 +2,7 @@ module AttributeNormalizer | |||
| module Normalizers | |||
| module SquishNormalizer | |||
| def self.normalize(value, options = {}) | |||
| value.is_a?(String) ? value.strip.gsub(/\s+/, ' ') : value | |||
| value.is_a?(String) ? value.strip.squeeze : value | |||
There was a problem hiding this comment.
irb> " hello\t world".strip.gsub /\s+/, ' '
=> "hello world"
irb> " hello\t world".strip.squeeze
=> "helo\t world"
There was a problem hiding this comment.
You're making an excellent point: the behavior of this normalizer regarding tabs & newlines isn't specified (only tested with normal spaces in specs), nor clearly documented:
- :squish Will strip leading and trailing whitespace and convert any consecutive spaces to one space each
I was personally using this normalizer on varchar columns (usually represented with a simple <input> field, thus not really facing the tab/newline problem), but as soon as I went using it on text columns (with <textarea>s), I realized it was dropping newlines, a behavior that didn't suit my needs (basic simple_format), and that I personally found surprising, and deceptive according to the documentation.
IMHO, either the documentation & specs have to be updated accordingly (which makes this normalizer almost useless for multilines attributes), or the implementation has to be modified to follow the documentation (which will not be backward compatible).
BTW, maybe another normalizer could be introduced for multiline attributes, to remove all control characters except for newlines:
config.normalizers[:remove_control_characters] = lambda do |value, options|
value.try(:gsub, /[[:cntrl:]&&[^\n]]/, '')
endAnyway, I'd be happy to help :)
There was a problem hiding this comment.
Also:
irb> " hello\t world".strip.squeeze
=> "helo\t world"
Note the missing l.
There was a problem hiding this comment.
Oh sorry about that I've totally missed the ' ' argument and thought the missing l was a typo.
I've corrected this in 748e2bd which is included in the PR.
I've also updated the original benchmarks to reflect this change
…ing spec to avoid regression
|
ActiveSupport provides a I am not in favor of changing this normalizer to use |
|
Oh, sorry about that, I've never really came across this ActiveSupport |
According to these benchmarks, using
squeezeis an order of magnitude faster thangsubwhen dealing with squishing/squeezing spaces on almost all Ruby implementations, except on Rubinius where it doesn't make much of a difference.Here is a patch that replaces
gsubbysqueezeinSquishNormalizer.normalize, with all tests green :)